Skip to content

test/fuzzer: fix AFL harness input handling and restore it to a compiling state - #3607

Open
shotintoeternity wants to merge 2 commits into
owasp-modsecurity:v3/masterfrom
shotintoeternity:fix/afl-fuzzer-input-handling
Open

test/fuzzer: fix AFL harness input handling and restore it to a compiling state#3607
shotintoeternity wants to merge 2 commits into
owasp-modsecurity:v3/masterfrom
shotintoeternity:fix/afl-fuzzer-input-handling

Conversation

@shotintoeternity

@shotintoeternity shotintoeternity commented Aug 1, 2026

Copy link
Copy Markdown

what

Restores test/fuzzer/afl_fuzzer.cc to a compiling state and makes it fuzz the
actual input it is given.

  • Builds the input string from buf instead of the fill constructor, and skips
    iterations where read returns nothing.
  • Updates op_test to the current 3-argument Operator::evaluate.
  • Updates the 37 generated transformation calls to the current
    Transformation::transform(std::string&, const Transaction*).
  • Adds a single-shot __AFL_LOOP fallback so the harness compiles without afl++
    installed.

One file, no build-system or CI changes.

why

The harness does not compile. On current v3/master:

$ c++ -fsyntax-only -std=c++17 -D'__AFL_LOOP(x)=1' \
      -I. -Iheaders -Isrc -Iothers test/fuzzer/afl_fuzzer.cc
38 errors, all "no matching member function for call to 'evaluate'"

37 are the generated transformation lines and 1 is op_test. __AFL_LOOP is
defined on the command line above because afl-clang-fast supplies it, so it is
not one of the errors.

Two API changes caused this, neither of which touched the harness:

  • 5d398907 (merged 2024-08-27) renamed Transformation::evaluate to
    transform(std::string &value, const Transaction *trans) and removed
    Action::evaluate(const std::string&, Transaction*).
  • 4df297b5 (merged 2024-10-07) changed the fourth parameter of
    Operator::evaluate from std::shared_ptr<RuleMessage> to RuleMessage&, so
    nullptr no longer binds.

Syntax-checking 5d398907^ gives 0 errors, so the harness last built in August
2024. It is gated behind --enable-afl-fuzz, which defaults to false
(configure.ac:284) and only then generates test/fuzzer/Makefile
(configure.ac:386), so no CI job surfaces the breakage.

The input was never the fuzzer's input. Independently of the above,
std::string(read_bytes, 128) selects the fill constructor
basic_string(size_type count, CharT ch), giving read_bytes copies of the
character 128. Overload resolution is not ambiguous, which is likely why it
survived: the pointer-and-length constructor is not viable because ssize_t does
not convert to const char*, and the iterator-pair constructor is not viable
because deduction conflicts. buf appears three times in the file, in its
declaration, its memset and its read, and is never read from. The compiler has
been saying so all along:

warning: implicit conversion from 'int' to 'char' changes value
         from 128 to -128 [-Wconstant-conversion]

git log -L145,145:test/fuzzer/afl_fuzzer.cc returns a single commit, c2d9a153
("Adds support to afl fuzzer in the build system", 2015-12-22), so the line is
unchanged since the harness was written.

The practical effect is that the harness's input domain was a run of 0x80 bytes
whose only attacker-controlled property was its length, 0 to 128, which is 129
distinct inputs. t:hexDecode never saw a hex digit, Base64Decode never saw
base64, DetectSQLi and DetectXSS never saw a quote or an angle bracket.

on the transformation calls

The 37 transformation lines are ported to a helper that mirrors the existing
op_test:

template <typename T>
inline void tfn_test(const std::string &tfnName, const std::string &s,
    const Transaction *t) {
    T tfn(tfnName);
    // transform() rewrites in place, so each transformation gets its own copy
    // of the fuzzer input rather than the output of the previous one.
    std::string value(s);
    tfn.transform(value, t);
}

so each generated line becomes tfn_test<Base64Decode>("Base64Decode", s, t);.

This constructs the classes directly, as the current file already does, rather than
routing through Transformation::instantiate. That is deliberate.
instantiate matches with

#define IF_MATCH(b) \
    if (a.compare(2, std::strlen(#b), #b) == 0)

The compare starts at offset 2 because it expects the rule-language spelling with
the t: prefix, as in t:base64Decode. Class-cased names match nothing, fall
through every branch, and reach the final return new Transformation(a), which is
the base class, whose transform() returns false without touching the value.
Passing the 37 class names to instantiate and checking the dynamic type:

Base64Decode         dynamic_type=Transformation  transform_changed=NO
Base64DecodeExt      dynamic_type=Transformation  transform_changed=NO
HexDecode            dynamic_type=Transformation  transform_changed=NO
...
base-class no-ops: 37   real transformations: 0

control, rule-language spelling with the t: prefix:
t:hexDecode          dynamic_type=<subclass>      transform_changed=yes
t:lowercase          dynamic_type=<subclass>      transform_changed=yes
t:urlDecode          dynamic_type=<subclass>      transform_changed=yes

Direct construction sidesteps the prefix convention entirely and keeps the name
list compile-checked, so a wrong name is a build error rather than a silent no-op.

on the __AFL_LOOP fallback

afl-clang-fast defines __AFL_LOOP. Without it the file does not compile, which
is part of why this rot went unnoticed for two years. The fallback here is
single-shot rather than a literal 1, so a non-AFL build reads stdin once and
exits instead of spinning at EOF against the new read_bytes <= 0 guard.

verification

Built libmodsecurity from this branch and compiled the harness against it with a
plain compiler:

  • 0 errors. The only warnings are the 4 pre-existing -Wcomment ones from the /*
    sequences inside the "generated by" block comments, which are untouched here.
  • Runs on real stdin input and exits 0.
  • Exits 0 on empty input rather than looping.

credit

The input-handling bug and the op_test signature fix are @Easton97-Jens's find,
from #3556. This PR carries the afl_fuzzer.cc portion of that work on its own, at
their suggestion, so it can be reviewed without the CI and workflow changes bundled
there. The CI additions in #3556 are unaffected by this and remain for separate
discussion.

Summary by CodeRabbit

  • Tests
    • Improved fuzz-testing reliability across environments where AFL loop support is unavailable.
    • Added safe handling for empty, invalid, or nonpositive input reads.
    • Ensured each transformation is tested with independent input data.
    • Updated test execution to use the current evaluation interface.
    • Improved generated test setup for more consistent fuzzing across supported environments.

…ling state

The harness has not compiled since 5d39890 (merged 2024-08-27) renamed
Transformation::evaluate to transform(std::string&, const Transaction*) and
removed Action::evaluate(const std::string&, Transaction*). 4df297b (merged
2024-10-07) later changed the fourth parameter of Operator::evaluate from
std::shared_ptr<RuleMessage> to RuleMessage&, breaking op_test as well. Neither
change touched test/fuzzer/, and --enable-afl-fuzz defaults to false
(configure.ac:284), so no CI job surfaced the breakage. A syntax check of
current v3/master reports 38 errors in the file.

Independently of that, std::string(read_bytes, 128) selected the fill
constructor basic_string(size_type count, CharT ch), so the harness ran against
read_bytes copies of the character 128 rather than the input AFL supplied. buf
was never read from. The compiler has been reporting this as a
-Wconstant-conversion warning since the harness was written in c2d9a15.

- Build the input string from buf, and skip iterations where read returns
  nothing.
- Update op_test to the current 3-argument Operator::evaluate.
- Port the 37 generated transformation calls to Transformation::transform,
  constructing the classes directly rather than via Transformation::instantiate,
  whose t:-prefixed matching does not accept class-cased names and would
  silently yield base-class no-ops.
- Add a single-shot __AFL_LOOP fallback so the harness compiles without afl++
  installed, and terminates rather than spinning on empty input.

Original find and the input-handling fix by @Easton97-Jens in owasp-modsecurity#3556.
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9a3acea2-b050-4856-a0a1-77f87fc603cd

📥 Commits

Reviewing files that changed from the base of the PR and between a096136 and 3e34aac.

📒 Files selected for processing (1)
  • test/fuzzer/afl_fuzzer.cc
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/fuzzer/afl_fuzzer.cc

📝 Walkthrough

Walkthrough

The AFL harness adds a single-iteration fallback, validates read results, uses the actual input length, and evaluates transformations through a transaction-aware helper with isolated input copies.

Changes

AFL harness execution

Layer / File(s) Summary
Input execution and read handling
test/fuzzer/afl_fuzzer.cc
The harness runs one iteration when __AFL_LOOP is unavailable. It skips empty or failed reads and builds the input from the bytes read.
Transaction-aware transformation flow
test/fuzzer/afl_fuzzer.cc
The harness uses tfn_test for isolated transformation inputs and the updated evaluate signature. The documented include-generation commands use find.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: restoring compilation and correcting AFL harness input handling.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

The generator commands recorded in the harness comments contained
src/actions/transformations/*.h and similar, so a "/*" sat inside a block
comment and reads as a nested comment opener. SonarQube reports it as
cpp:S1103.

Quote the glob when handing it to find, and pass the directory straight to
grep -R, which needs no glob at all. Both rewritten forms produce output
byte-identical to the originals, so the comments still document exactly what
generated the include list and the tfn_test/op_test calls.

Comment-only change. The harness behavior is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Aug 1, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant